Skip to content

fix(chat-frontend): sidebar bucket fetch + always-render section headers#188

Merged
mliu33 merged 10 commits into
mainfrom
claude/sidebar-flat-list-fallback
May 18, 2026
Merged

fix(chat-frontend): sidebar bucket fetch + always-render section headers#188
mliu33 merged 10 commits into
mainfrom
claude/sidebar-flat-list-fallback

Conversation

@GITMateuszCharczuk
Copy link
Copy Markdown
Collaborator

@GITMateuszCharczuk GITMateuszCharczuk commented May 15, 2026

Symptom

On the real user-service: after PR #181 (three-section sidebar), users who previously saw 3 rooms in the sidebar now see 0 rooms on cold start. Creating a new room makes it appear live, but a refresh wipes it again. No red errors in the console — just a yellow console.warn you'd have to be looking for.

Root cause

Two distinct problems stacked together:

1. Wrong endpoint for the Channels and DMs bucket

PR #181 wired fetchSidebarBuckets to call three user-service RPCs in parallel:

request(userSubscriptionGetCurrent(...), { favorite: true })  // favorites
request(userSubscriptionGetApps(...), {})                     // apps
request(userSubscriptionGetRooms(...), {})                    // channels + DMs

subscription.getRooms does exist as a subject (pkg/subject/subject.go:492) and is wired on both mock-user-service and the real user-service. But on the real backend, it doesn't return the user's normal channel/DM subscriptions — only subscription.getCurrent (no payload) does.

2. Strict bucket partitioning silently drops un-bucketed rooms

useSidebarSections partitions state.summaries strictly by favoriteIds / appIds / channelDmIds:

for (const room of summaries) {
  if (favoriteIds.has(room.id))   favorite.push(...)
  else if (appIds.has(room.id))   apps.push(...)
  else if (channelDmIds.has(room.id)) channelDm.push(...)
  // un-bucketed rooms → DROPPED
}

RoomList.jsx then renders nothing for any section with rooms.length === 0. So even though listRooms correctly populated state.summaries with the user's 3 rooms, none of those IDs were in any bucket Set, and the entire sidebar rendered blank.

The failure was swallowed by:

.catch((err) => console.warn('sidebar bucket bootstrap failed:', err?.message ?? err))

A console.warn, not an error — doesn't trip ErrorBoundary or surface as red in DevTools.

Fix (three commits)

6bbe9db — empty-bucket flat-list fallback

useSidebarSections: when all three bucket Sets are empty, push every summary into Channels and DMs as a flat list. Safety net for any future RPC failure; preserves strict partitioning whenever any bucket has content.

6556eb1 — drop getRooms, use getCurrent() as canonical fetch

fetchSidebarBuckets now hits 2 endpoints (3 calls):

  • getCurrent() (no payload) — canonical full subscription list, feeds both state.subscriptions (source of truth for useSubscription) and channelDmIds
  • getCurrent({ favorite: true }) — favorited IDs for the Favorite section
  • getApps() — kept for the Apps section

The userSubscriptionGetRooms subject builder is dropped from api/_transport/subjects.ts.

Render-time partition stays favorite > apps > channelDm, so a favorited app/channel still appears only under Favorite even though its ID is in channelDmIds too.

343b855 — always render all three section headers

Favorite / Apps / Channels and DMs each render with their collapsible header even when the bucket is empty, so the user always sees the sidebar structure. Adds a chevron () in every header that rotates 90° via CSS when collapsed, and an italic "No rooms" placeholder under an empty expanded section.

1e65860 — TEMP debug logging

console.log the three subscription RPC replies (subject + response) on cold start to verify the real user-service behaviour. Remove before final merge.

Why listRooms AND subscription.getCurrent both run on cold start

They carry different data:

  • listRooms (rooms.list) returns canonical Room records — userCount, lastMsgAt, canonical room name, type. Populates state.summaries.
  • subscription.getCurrent returns the user's per-room state — roles, hasMention, alert, lastSeenAt, hrInfo for DMs, subscription-local name override. Populates state.subscriptions (source of truth for useSubscription) and the bucket Sets.

mergeSubscriptionIntoSummary joins the two at render time. Subscriptions alone can't drive the sidebar because they don't carry userCount / lastMsgAt; listRooms alone can't carry per-user state like role / mention / DM friend metadata.

Test plan

  • Cold start on the real user-service — Favorite / Apps / Channels and DMs all render, 3 rooms appear under Channels and DMs
  • Verify the three [sidebar-bootstrap] console logs show the expected replies
  • Create a new room — appears live in the correct section
  • Refresh — room persists
  • User with zero rooms — all three section headers still visible with "No rooms" placeholders
  • Collapse / expand a section via header click — chevron rotates, items hide/show
  • Remove the TEMP debug logging in a follow-up before final merge

506/506 tests passing, typecheck clean.

Summary by CodeRabbit

  • New Features

    • Sidebar section headers are now collapsible with chevrons and can display an optional note instead of an empty placeholder.
    • Live role updates are applied immediately when subscription changes arrive.
  • Bug Fixes

    • All three sidebar sections always render; empty sections show a "No rooms" placeholder when appropriate.
    • Sidebar bootstrapping now degrades gracefully when some subscription data fails, still showing available rooms.

Review Change Stack

useSidebarSections partitions summaries strictly by favoriteIds /
appIds / channelDmIds. listRooms is independent of the three
subscription.get* RPCs that populate those Sets, so if the bucket
RPCs fail or haven't landed yet, every room in `summaries` is
dropped by the partition and the sidebar renders blank — the user
sees an "empty page" with no errors, just a console.warn from the
fetchSidebarBuckets catch.

Cold-start safety: when all three bucket Sets are empty, push every
summary into Channels and DMs as a flat list. The next
BUCKETS_LOADED dispatch re-partitions normally. Strict partitioning
is preserved whenever any bucket has content.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

📝 Walkthrough

Walkthrough

Sidebar bootstrap now collects three subscription RPCs (getCurrent/getApps/getRooms) via fetchSidebarBuckets, merges subscriptions into per-room summaries, and returns room records; useSidebarSections falls back to listing unmatched rooms under Channels and DMs during cold start; RoomList always renders section headers with chevrons and empty-state/note handling; reducer/hooks/types/tests updated to support subscription update events and idempotency.

Changes

Sidebar bootstrap and bucket fetch

Layer / File(s) Summary
Transport subjects and fetchSidebarBuckets
chat-frontend/src/api/fetchSidebarBuckets/index.ts, chat-frontend/src/api/types.ts, chat-frontend/src/api/index.ts
Rewrite fetchSidebarBuckets to call subscription.getCurrent/getApps/getRooms via Promise.allSettled, merge subscription responses into a per-room map, derive favoriteIds, appIds, channelDmIds and a rooms: Room[] list, and add room-derivation helper; add optional room metadata to Subscription and new SubscriptionUpdateAction/SubscriptionUpdateEvent types re-exported from @/api.
subscribeToSubscriptionUpdates and transport docs/tests
chat-frontend/src/api/subscribeToSubscriptionUpdates/index.ts, chat-frontend/src/api/_transport/subjects.ts, chat-frontend/src/api/_transport/subjects.test.js
Add SubscriptionUpdateCallback type and narrow subscribeToSubscriptionUpdates callback typing, cast at the transport boundary; update inline subject docs and minor subjects test whitespace adjustments.

RoomList rendering, tests, and styling

Layer / File(s) Summary
RoomList render and header
chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx
Always render section wrappers/headers (no early return for empty sections); include a chevron indicator element in headers; when expanded, render a "No rooms" placeholder only if section.rooms is empty and section.note is absent.
RoomList tests and interaction
chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.test.jsx
Normalize header text by stripping a leading chevron glyph in assertions; assert all three section headers render even when empty; verify "No rooms" placeholders and that a provided note replaces the placeholder; ensure each header renders a chevron and toggles collapsed state on click.
Styles and gitignore
chat-frontend/src/components/MainApp/Sidebar/RoomList/style.css, .gitignore
Add flex layout and gap for section header, chevron transition and collapsed rotation, italic/spacing styles for empty/note rows, and ignore Vitest cache/JUnit artifacts in .gitignore.

RoomEventsContext logic and tests

Layer / File(s) Summary
useSidebarSections partition & SidebarSection shape
chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
Add note?: string to SidebarSection; compute allBucketsEmpty and, when true, assign unmatched summaries into channelDm so the sidebar still lists rooms during bucket bootstrap failures/delays.
Reducer enrichment and idempotency
chat-frontend/src/context/RoomEventsContext/reducer.js, chat-frontend/src/context/RoomEventsContext/reducer.test.js
On ROOMS_LOADED, merge any existing per-room subscription record into summaries; on ROOM_ADDED, short-circuit and return original state if summaries/appIds/channelDmIds identities are unchanged; BUCKETS_LOADED now optionally rebuilds summaries from action.rooms or enriches existing summaries. Tests added for live-add race, ROOM_ADDED idempotency, and BUCKETS-before-ROOMS enrichment.
useRoomSubscriptions and RoomEventsContext tests
chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js, chat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsx
Remove legacy listRooms bootstrap; use fetchSidebarBuckets for initial bootstrap and open channel subscriptions for returned rooms; on subscription update events that include evt.subscription.roomId, dispatch SUBSCRIPTION_UPSERTED to upsert subscription metadata; update provider tests to stub subscription.getCurrent/getApps/getRooms RPC shapes, add live role_updated propagation tests, and adapt many tests to the new bucket bootstrap model.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • hmchangw/chat#181: Introduces the useSidebarSections hook and earlier sidebar section behavior this PR adjusts.
  • hmchangw/chat#105: Related earlier work on the room event dispatcher/context wiring that this PR continues to evolve.
  • hmchangw/chat#178: Touches room summary enrichment and subscription-derived metadata similar to this PR.

Suggested reviewers

  • mliu33
  • hmchangw

Poem

🐰 A rabbit peeks where buckets hum and wake,
It nudges rooms back in for sidebar's sake,
Chevrons winking, notes that softly show,
Live roles hop in — the list will calmly grow.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.94% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main fixes: sidebar bucket fetch changes and always-rendering section headers, which directly address the cold-start regression discussed in PR objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/sidebar-flat-list-fallback

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

claude added 2 commits May 17, 2026 15:33
`subscription.getRooms` exists as a subject (pkg/subject/subject.go)
and is wired on both mock-user-service and the real user-service, but
on the real backend it doesn't return the user's normal channel/DM
subscriptions — only `getCurrent` (no payload) does. Without a
populated `channelDmIds` Set, `useSidebarSections`'s strict
partitioning dropped every room from `listRooms` after a refresh,
leaving the sidebar blank.

Bootstrap now hits two endpoints:
  - `getCurrent()` (no payload) — canonical full subscription list,
    feeds `state.subscriptions` (source of truth for `useSubscription`)
    and seeds `channelDmIds`. Channels and DMs are partitioned at
    render time from this single list, by roomType.
  - `getCurrent({ favorite: true })` — favorited IDs for the Favorite
    section.
  - `getApps()` — kept for the Apps section.

The empty-bucket fallback from the previous commit stays as a safety
net for any future RPC failure, but the normal path now actually
populates the buckets.

Also: gitignore `chat-frontend/junit.xml` (vitest CI reporter output).
Favorite / Apps / Channels and DMs each render with their collapsible
header even when the bucket is empty, so the user always sees the
sidebar structure. Adds a chevron (▾) in every header that rotates
90° via CSS when the section is collapsed, and an italic "No rooms"
placeholder under an empty expanded section.

Before: empty sections were hidden entirely (return null) — if you
had no favorites you couldn't see the Favorite header at all, and a
user with zero rooms saw a completely blank sidebar.
@GITMateuszCharczuk GITMateuszCharczuk force-pushed the claude/sidebar-flat-list-fallback branch from f1fbde5 to 343b855 Compare May 17, 2026 15:35
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx (1)

47-53: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use a semantic button for collapsible section headers.

The collapsible header is click-only on a div, so keyboard users can’t reliably operate it. Make it a button (or add full keyboard semantics) to avoid an accessibility blocker.

Proposed fix
-              <div
+              <button
+                type="button"
                 className="room-list-section-header"
                 onClick={() => toggle(section.key)}
               >
                 <span className="room-list-section-chevron" aria-hidden="true">▾</span>
                 {section.title}
-              </div>
+              </button>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx` around
lines 47 - 53, The collapsible header currently uses a click-only div (className
"room-list-section-header") which is inaccessible; replace that div with a
semantic <button> (or React button element) inside RoomList.jsx and attach the
existing onClick handler (toggle(section.key)) to it, add
aria-expanded={isExpanded} (compute from your section state) and aria-controls
pointing to the collapsible region id, keep the chevron span
(aria-hidden="true") and the section.title inside the button, and remove any
custom keyboard handlers since a native button provides proper keyboard
semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 50-56: The fetchSidebarBuckets API currently calls request(...)
with two args; update the function signature for fetchSidebarBuckets to accept
an optional opts parameter and pass that opts as the third argument to all three
request calls (the requests using subjects userSubscriptionGetCurrent and
userSubscriptionGetApps) so each request uses request(subject, payload, opts)
consistently; ensure the opts parameter is threaded through even when undefined
to match API/test conventions.
- Around line 50-57: The Promise.all call can throw away the successful
canonical payload (allResp) if either the favorites or apps requests reject;
change the logic to run the three requests with Promise.allSettled (or await
each with try/catch) so that userSubscriptionGetCurrent(user.account,
user.siteId) is preserved even when the other two fail, then map rejected
results for the favorite and apps calls to safe empty SidebarBucketReply
fallbacks and continue to use allResp to seed state.subscriptions and
channelDmIds; update references to allResp, favResp, and appResp accordingly so
only the failed auxiliaries are ignored rather than aborting the whole
bootstrap.

---

Outside diff comments:
In `@chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx`:
- Around line 47-53: The collapsible header currently uses a click-only div
(className "room-list-section-header") which is inaccessible; replace that div
with a semantic <button> (or React button element) inside RoomList.jsx and
attach the existing onClick handler (toggle(section.key)) to it, add
aria-expanded={isExpanded} (compute from your section state) and aria-controls
pointing to the collapsible region id, keep the chevron span
(aria-hidden="true") and the section.title inside the button, and remove any
custom keyboard handlers since a native button provides proper keyboard
semantics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 520dea54-8b82-4a4f-bd5d-c4c4cb2d0294

📥 Commits

Reviewing files that changed from the base of the PR and between 6bbe9db and 343b855.

📒 Files selected for processing (8)
  • .gitignore
  • chat-frontend/src/api/_transport/subjects.test.js
  • chat-frontend/src/api/_transport/subjects.ts
  • chat-frontend/src/api/fetchSidebarBuckets/index.ts
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.test.jsx
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/style.css
  • chat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsx
💤 Files with no reviewable changes (1)
  • chat-frontend/src/api/_transport/subjects.test.js

Comment thread chat-frontend/src/api/fetchSidebarBuckets/index.ts Outdated
Comment thread chat-frontend/src/api/fetchSidebarBuckets/index.ts Outdated
console.log each of the three subscription bootstrap RPCs (the two
getCurrent calls and getApps) with their subject + response so we
can verify what the real user-service returns. Remove once the live
backend behaviour is confirmed.
@GITMateuszCharczuk GITMateuszCharczuk changed the title fix(chat-frontend): fall back to flat sidebar when bucket RPCs fail fix(chat-frontend): sidebar bucket fetch + always-render section headers May 17, 2026
claude added 4 commits May 17, 2026 15:46
Pairs with the existing [sidebar-bootstrap] logs in fetchSidebarBuckets
so we can diff the listRooms reply against the subscription RPC
replies. A room that appears in listRooms but not in any of the three
subscription buckets is silently dropped by useSidebarSections's
strict partition (unless every bucket is empty, in which case the
all-empty fallback catches it). The diff in console makes that gap
visible.

Remove once the live backend behaviour is verified.
…ction

Two changes addressing reviewer findings on the subscription pipeline:

1. fetchSidebarBuckets now uses Promise.allSettled so a single
   subscription RPC failure no longer black-holes the entire sidebar
   bootstrap (Reviewer 3 H1). Each rejection is logged with its subject
   and the bucket degrades to empty rather than rejecting the whole
   bucket fetch.

2. The favorite RPC (`getCurrent {favorite:true}`) is soft-disabled —
   the call is commented out (not deleted, so re-enabling is one line)
   and `useSidebarSections` now marks the Favorite section with a
   `note: 'Favorites are not yet supported'`. RoomList renders the
   note in place of room items / empty placeholder. Rationale:
   `pkg/model.Subscription` has no Favorite field today (Reviewer 1
   H2 / Reviewer 2 H2), so the filter is unenforceable end-to-end and
   we'd be misleading the user by pretending it works.

The Favorite section header still renders (collapsible, with chevron)
for layout continuity. Clear the note + uncomment the RPC once the
favorite path lands on the backend.

Tests updated: the bucket-bootstrap test now asserts 3 calls (was 4),
the failure-tolerance test exercises a failing getApps with getCurrent
still succeeding, and the favorite-exclusivity test asserts the
section stays empty regardless of mocked favoriteIds.
…removed

useRoomSubscriptions previously only branched on `added` and `removed`,
so live `role_updated` events from room-worker (`handler.go:197`) were
silently dropped. An admin demoting / promoting a user mid-session left
the frontend with stale `state.subscriptions[r].roles` until reload,
which manifested as the user still seeing owner-only UI affordances
they no longer had access to.

The handler now adds a catch-all branch: any subscription.update event
that carries a `subscription.roomId` and isn't add/remove dispatches
SUBSCRIPTION_UPSERTED. The reducer already partial-merges so a payload
with only some fields won't drop lastSeenAt / hasMention / alert from
the prior record. Forward-compatible with mute / favorite / mark-read
the moment those backend paths start publishing events.

Closes Reviewer 4 H2.
Three reviewer-flagged issues, all in the same blast radius:

1. Mirror SubscriptionUpdateEvent on the TS side (Reviewer 3 M2).
   subscribeToSubscriptionUpdates's callback was typed
   (event: unknown) => void — every consumer had to hand-narrow.
   New types: SubscriptionUpdateEvent + SubscriptionUpdateAction (union
   ∪ string for forward compat) in api/types.ts; callback re-typed to
   the parsed event. Re-exported from the api barrel.

2. ROOMS_LOADED race with BUCKETS_LOADED (Reviewer 4 M1). When the
   parallel cold-start fetches resolved bucket-first, the subsequent
   ROOMS_LOADED replaced state.summaries via toSummary's zero-init,
   wiping the server-canonical hasMention / subscriptionName that
   BUCKETS_LOADED had just seeded into state.subscriptions. The
   reducer now enriches each incoming summary via
   mergeSubscriptionIntoSummary using whatever's already in
   state.subscriptions — order-independent.

3. ROOM_ADDED early-return skipped bucket-Set maintenance (Reviewer 4
   H3). If listRooms returned the room first (ROOMS_LOADED seeds it
   into summaries) and a live `subscription.update added` then
   arrived, the existing-summary check at the top of the case bailed
   out — leaving the room in summaries but in NO bucket Set. The
   strict partition in useSidebarSections then silently dropped it
   (until the all-empty fallback kicks in, which only catches the
   total-empty case). The case now always runs bucket maintenance;
   summary append is skipped when the row already exists. An
   object-identity short-circuit at the end keeps the true no-op
   (duplicate event, room already in correct bucket) from triggering
   re-renders.

Reducer tests added for both races; the no-op idempotency contract
is also pinned (`after2 === after1`).
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
chat-frontend/src/api/fetchSidebarBuckets/index.ts (1)

65-67: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Thread opts through NATS requests in this API operation.

These calls still use the 2-argument request shape. Please add optional opts to fetchSidebarBuckets and pass request(subject, payload, opts) consistently.

As per coding guidelines, "Always pass opts parameter through to NATS primitives in API operations, even when undefined — use three-argument shape for consistency with tests".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts` around lines 65 - 67, Add
an optional opts parameter to fetchSidebarBuckets and thread it into the NATS
requests: change the two-argument calls to request(currentSubject, {}, opts) and
request(appsSubject, {}, opts) (use the three-argument request(subject, payload,
opts) shape) so both Promise.allSettled entries pass opts consistently; update
the fetchSidebarBuckets signature to accept opts (optional) and forward it to
any other request(...) invocations in that function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 34-42: The docblock is out of sync: it claims getCurrent({
favorite: true }) is fetched for Favorites but that code path is disabled;
update the comment around the bootstrap description to reflect the actual
runtime flow (only getCurrent() is fetched as the canonical subscription list
which seeds state.subscriptions and channelDmIds, and getApps() is fetched for
the Apps section) and note that Favorites are derived from state.subscriptions
at render time (or that favorite-specific fetching is intentionally disabled)
instead of claiming getCurrent({ favorite: true }) is called.
- Around line 75-79: Remove the temporary debug that logs full subscription
payloads: delete the console.log('[sidebar-bootstrap]', label, result.value)
call in the fetch/sidebar bootstrap code so that label and result.value are not
emitted to logs; keep the return result.value behavior unchanged, or if you need
retained diagnostics replace it with a sanitized/logger-guarded message (e.g.,
log only label or a redacted summary) using the existing logger instead of raw
console.log.

In `@chat-frontend/src/api/listRooms/index.ts`:
- Around line 10-11: Update the listRooms API to accept an optional opts
parameter and pass it through to the NATS request call: add an opts parameter to
the function signature where roomsList(user.account) / subject is created, and
call request<ListRoomsResponse>(subject, {}, opts) (three-argument form) instead
of the current two-argument call; ensure you preserve the payload shape and
types and thread opts through even when undefined to satisfy tests and coding
guidelines.
- Around line 12-16: Remove the temporary debug logging in the listRooms API
handler: delete the console.log call that prints '[sidebar-bootstrap]' along
with subject and resp (located in chat-frontend/src/api/listRooms/index.ts
inside the listRooms function or exported handler) so the full rooms response
payload is not logged; ensure no other leftover debug console.* calls remain and
return resp unchanged.

---

Duplicate comments:
In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 65-67: Add an optional opts parameter to fetchSidebarBuckets and
thread it into the NATS requests: change the two-argument calls to
request(currentSubject, {}, opts) and request(appsSubject, {}, opts) (use the
three-argument request(subject, payload, opts) shape) so both Promise.allSettled
entries pass opts consistently; update the fetchSidebarBuckets signature to
accept opts (optional) and forward it to any other request(...) invocations in
that function.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 016dec88-e760-4f10-8ed1-0b9707beb82a

📥 Commits

Reviewing files that changed from the base of the PR and between 343b855 and 943d09c.

📒 Files selected for processing (13)
  • chat-frontend/src/api/fetchSidebarBuckets/index.ts
  • chat-frontend/src/api/index.ts
  • chat-frontend/src/api/listRooms/index.ts
  • chat-frontend/src/api/subscribeToSubscriptionUpdates/index.ts
  • chat-frontend/src/api/types.ts
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.jsx
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.test.jsx
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/style.css
  • chat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsx
  • chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
  • chat-frontend/src/context/RoomEventsContext/reducer.js
  • chat-frontend/src/context/RoomEventsContext/reducer.test.js
  • chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js
🚧 Files skipped from review as they are similar to previous changes (3)
  • chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/RoomList.test.jsx
  • chat-frontend/src/components/MainApp/Sidebar/RoomList/style.css

Comment thread chat-frontend/src/api/fetchSidebarBuckets/index.ts Outdated
Comment thread chat-frontend/src/api/fetchSidebarBuckets/index.ts Outdated
Comment thread chat-frontend/src/api/listRooms/index.ts Outdated
Comment thread chat-frontend/src/api/listRooms/index.ts Outdated
Two pre-test-env tweaks from the final reviewer round:

1. Favorite section's `note: 'Favorites are not yet supported'` is now
   set only when `favorite.length === 0`. The favorite RPC is still
   commented out so favoriteIds stays empty in production today, but
   if anything ever lands a roomId in favoriteIds (re-enabled RPC,
   live event), the section will render the rooms instead of silently
   hiding them behind the unsupported-note. Belt-and-braces against a
   reviewer-flagged HIGH that would have manifested as missing rooms
   when the favorite path is eventually wired up.

2. TEMP `[sidebar-bootstrap]` console.log payloads compacted from full
   reply objects to `{ count, roomIds }`. Accounts with many rooms
   used to produce a wall of JSON per RPC; now each log line fits on
   a screen and shows exactly the data needed to diff the
   subscription RPCs against listRooms. Still TEMP — remove after the
   live backend behaviour is verified.
Copy link
Copy Markdown
Collaborator

@mliu33 mliu33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what we are trying to fix in this PR ?

Comment thread chat-frontend/src/api/fetchSidebarBuckets/index.ts
Comment thread chat-frontend/src/api/_transport/subjects.ts
Comment thread chat-frontend/src/api/listRooms/index.ts Outdated
Addresses three review comments from mliu33 on PR #188:
  - "These are actually correct in real user-service" (re: the three
    `subscription.get*` RPCs)
  - "This should not get dropped" (re: `userSubscriptionGetRooms`
    subject builder)
  - "listRooms should be dropped because all three get-user-subscriptions
    rpc will return room info in the response (from real user service)"

**What changed**

Sidebar bootstrap now runs three subscription RPCs (the original
shape from PR #181, before this branch's `getCurrent`-canonical
detour):
  - `subscription.getCurrent { favorite: true }` → favorites bucket
  - `subscription.getApps {}` → apps bucket
  - `subscription.getRooms {}` → channels/DMs bucket

Each reply embeds room metadata (userCount, lastMsgAt, lastMsgId,
siteId) inline on every subscription record — the real user-service
joins room data into the response, so a separate `rooms.list` RPC is
not needed.

  - Restored `userSubscriptionGetRooms` subject builder in
    `_transport/subjects.ts` (and its test).
  - `fetchSidebarBuckets` rewritten: 3 RPCs via Promise.allSettled
    (per-bucket degradation preserved), unions the subscriptions, and
    returns a derived `rooms[]` for the reducer.
  - `state.subscriptions` is the canonical source of truth for both
    per-room state AND for building summaries.
  - `Subscription` TS type extended with optional `userCount`,
    `lastMsgAt`, `lastMsgId` to mirror the wire shape the real
    user-service emits.
  - `BUCKETS_LOADED` reducer case now constructs summaries from
    `action.rooms` when provided (cold-start path); falls back to the
    enrich-existing-summaries behaviour when `rooms` is omitted (test
    fixtures, future partial-refresh paths).
  - `listRooms` op deleted: `src/api/listRooms/index.ts` removed,
    barrel export gone, `useRoomSubscriptions` no longer dispatches
    `ROOMS_LOADED` (the reducer action stays as a primitive for tests
    and future use).
  - Favorite section: "Favorites are not yet supported" note removed
    — favorites are real on the live backend, so the section
    populates from `getCurrent { favorite: true }`.

**Tests**

47 files / 512 tests passing, typecheck clean. Test fixtures
migrated from `rooms.list` mocks to `subscription.getRooms` returning
room-bearing subscriptions via a new `roomToSub()` helper. The
bootstrap-fires-three-RPCs and Promise.allSettled-degradation tests
both refactored to the new wire shape.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
chat-frontend/src/api/fetchSidebarBuckets/index.ts (2)

76-81: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove temporary sidebar bootstrap debug logs before merge.

The cold-start console.log still emits per-user room identifiers. Keep failure logging if needed, but drop payload-style success logs.

Suggested fix
-    if (result.status === 'fulfilled') {
-      // TEMP DEBUG: compact summary so we can verify what each
-      // subscription RPC returns on cold start. Remove once verified.
-      console.log('[sidebar-bootstrap]', label, {
-        count: result.value.subscriptions.length,
-        roomIds: result.value.subscriptions.map((s) => s.roomId),
-      })
-      return result.value
-    }
+    if (result.status === 'fulfilled') {
+      return result.value
+    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts` around lines 76 - 81,
Remove the temporary debug console.log that prints per-user subscription
payloads in the sidebar bootstrap path; locate the log call that uses
'[sidebar-bootstrap]', label and result.value.subscriptions (and the mapped
roomIds) inside the fetchSidebarBuckets flow and delete that statement, leaving
only existing error/failure logging in place so success paths no longer emit
payload-style logs.

61-69: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Thread opts through this API operation and all NATS requests.

This operation still uses a 2-arg request(...) shape and does not accept { args, opts }, which breaks the API operation contract used across src/api/*/index.ts.

Suggested fix
-export async function fetchSidebarBuckets({ user, request }: Nats): Promise<SidebarBuckets> {
+export interface FetchSidebarBucketsArgs {}
+export interface FetchSidebarBucketsOpts {
+  timeout?: number
+}
+
+export async function fetchSidebarBuckets(
+  { user, request }: Nats,
+  _args: FetchSidebarBucketsArgs = {},
+  opts?: FetchSidebarBucketsOpts,
+): Promise<SidebarBuckets> {
@@
-    request<SidebarBucketReply>(favSubject, { favorite: true }),
-    request<SidebarBucketReply>(appsSubject, {}),
-    request<SidebarBucketReply>(roomsSubject, {}),
+    request<SidebarBucketReply>(favSubject, { favorite: true }, opts),
+    request<SidebarBucketReply>(appsSubject, {}, opts),
+    request<SidebarBucketReply>(roomsSubject, {}, opts),
   ])

As per coding guidelines, "chat-frontend/src/api/*/index.ts API operations must have the (..., args, opts?) signature and always pass opts to NATS primitives using the three-argument shape."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts` around lines 61 - 69,
fetchSidebarBuckets currently uses the 2-arg request(...) shape and doesn't
accept opts; update the function signature fetchSidebarBuckets({ user, request
}: Nats, args?, opts?) to follow the project's API contract and thread opts into
each NATS call: replace request<SidebarBucketReply>(favSubject, { favorite: true
}) and the two other request calls with the three-argument form
request<SidebarBucketReply>(favSubject, { favorite: true }, opts) (and likewise
for appsSubject and roomsSubject) and ensure Promise.allSettled still collects
those returned promises.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@chat-frontend/src/api/fetchSidebarBuckets/index.ts`:
- Around line 76-81: Remove the temporary debug console.log that prints per-user
subscription payloads in the sidebar bootstrap path; locate the log call that
uses '[sidebar-bootstrap]', label and result.value.subscriptions (and the mapped
roomIds) inside the fetchSidebarBuckets flow and delete that statement, leaving
only existing error/failure logging in place so success paths no longer emit
payload-style logs.
- Around line 61-69: fetchSidebarBuckets currently uses the 2-arg request(...)
shape and doesn't accept opts; update the function signature
fetchSidebarBuckets({ user, request }: Nats, args?, opts?) to follow the
project's API contract and thread opts into each NATS call: replace
request<SidebarBucketReply>(favSubject, { favorite: true }) and the two other
request calls with the three-argument form
request<SidebarBucketReply>(favSubject, { favorite: true }, opts) (and likewise
for appsSubject and roomsSubject) and ensure Promise.allSettled still collects
those returned promises.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e70d604-2ada-4247-9e9b-5d553137404c

📥 Commits

Reviewing files that changed from the base of the PR and between 943d09c and 9971dba.

📒 Files selected for processing (10)
  • chat-frontend/src/api/_transport/subjects.test.js
  • chat-frontend/src/api/_transport/subjects.ts
  • chat-frontend/src/api/fetchSidebarBuckets/index.ts
  • chat-frontend/src/api/index.ts
  • chat-frontend/src/api/listRooms/index.ts
  • chat-frontend/src/api/types.ts
  • chat-frontend/src/context/RoomEventsContext/RoomEventsContext.test.jsx
  • chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
  • chat-frontend/src/context/RoomEventsContext/reducer.js
  • chat-frontend/src/context/RoomEventsContext/useRoomSubscriptions.js
💤 Files with no reviewable changes (1)
  • chat-frontend/src/api/listRooms/index.ts
✅ Files skipped from review due to trivial changes (2)
  • chat-frontend/src/api/_transport/subjects.ts
  • chat-frontend/src/api/_transport/subjects.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • chat-frontend/src/context/RoomEventsContext/RoomEventsContext.tsx
  • chat-frontend/src/api/index.ts

@GITMateuszCharczuk GITMateuszCharczuk requested a review from mliu33 May 18, 2026 02:02
@mliu33 mliu33 merged commit 3c29a65 into main May 18, 2026
5 checks passed
GITMateuszCharczuk pushed a commit that referenced this pull request May 18, 2026
Actionable findings I applied:

**Frontend**
- Strip TEMP [thread-debug] logs from fetchThreadMessages and
  ThreadEventsContext.openThread — the bug is confirmed fixed in
  test env, the diagnostics served their purpose.
- editMessage / deleteMessage now pass response type through the
  generic `request<T>` (EditMessageResponse / DeleteMessageResponse)
  per the project's API contract; bodies define the wire shape even
  though the frontend swallows the response today.
- fanThreadReply in useRoomSubscriptions wraps the registered handler
  invocation in try/catch — a downstream consumer exception no
  longer breaks MESSAGE_RECEIVED dispatch or scheduleMarkActiveRead
  in the same callback.

**Backend**
- All three new ERROR/WARN logs (history-service threads.go +
  message-worker handler.go + store_cassandra.go) now include
  `request_id` from the context, per the root CLAUDE.md mandate
  ("propagate via context.Context, include in all log lines"). Lets
  the silent-stamp-miss log line correlate across handler → store →
  history-service for incident triage.

Skipped findings (with rationale):
- Adding `opts` parameter to fetchThreadMessages / editMessage /
  deleteMessage: `Nats.request` is a 2-arg primitive
  (`(subject, data?) => Promise<T>`) by design — the 3-arg opts
  contract applies to `requestWithAsyncResult` (async-job ops), not
  request-based ops. Sibling request ops (getRoom, listRoomMembers,
  markRoomRead, getUnreadCount, etc.) all use the 2-arg shape. Same
  argument as PR #188 review.
- Making UpdateParentMessageThreadRoomID return an error on
  IF EXISTS applied=false: the root cause that produced this miss
  (frontend optimistic createdAt) is fixed in this PR; converting
  the log to a hard error risks JetStream redelivery loops if any
  other timestamp-mismatch sneaks in. Worth doing as a separate
  defense-in-depth PR with retry/DLQ semantics designed alongside.

49 files / 525 frontend tests pass, go test ok on touched packages,
make lint clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants